Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix <em> issue with mixed content #1410 #1451

Merged
merged 11 commits into from
May 22, 2019
Merged

Conversation

x13machine
Copy link
Contributor

@x13machine x13machine commented Mar 18, 2019

Marked version:

Markdown flavor: Markdown.pl|CommonMark|GitHub Flavored Markdown|n/a

Description

It was caused _ and * parsed being differently in the em regex. I just replaced the part that searches for *test* and replaced it with the part searches for _test_ and then just charged the characters. I don't know why they were different. Maybe there's a reason for it. All the tests pass, so I dunno.

Contributor

  • Test(s) exist to ensure functionality and minimize regression

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@x13machine
Copy link
Contributor Author

ran wrong test command

@x13machine
Copy link
Contributor Author

x13machine commented Mar 18, 2019

Wait, is it expecting a bug to be there? If it is, I seemed to have fixed it.

@UziTech
Copy link
Member

UziTech commented Mar 18, 2019

Ya the spec tests that are currently failing will fail if they start passing. You will need to remove shouldFail: true from /test/specs/commonmark/commonmark.0.28.json line #3730

It looks like you are also failing some tests that used to pass.

@UziTech
Copy link
Member

UziTech commented Mar 18, 2019

The shouldFail lets us know which tests are fixed by each PR and let us keep track of the spec tests that we need to work on.

@x13machine
Copy link
Contributor Author

ok I figured out why they are different. It was to prevent it from being interpreted as an ordered List.

Also the underscore group has a bug: https://marked.js.org/demo/?text=_%3Cimg%20src%3D%22https%3A%2F%2Fwww.google.com%2Ffavicon.ico%22%20title%3D%22*_%22%2F%3E%0A%0A*%3Cimg%20src%3D%22https%3A%2F%2Fwww.google.com%2Ffavicon.ico%22%20title%3D%22*%22%2F%3E&options=&version=master

The asterisk was processed correctly. While the underscore was not. So the bug was transferred over.

So the underscore and asterisk groups need to be rewritten.

@x13machine
Copy link
Contributor Author

Ok I think, I fixed it this time.

lib/marked.js Outdated Show resolved Hide resolved
@UziTech
Copy link
Member

UziTech commented Mar 21, 2019

@davisjam could you check if these regexps are secure from redos?

Here are the new and old derived regexps for em:

New:

/^_([^\s_])_(?!_)|^\*([^\s*<\[])\*(?!\*)|^_([^\s<][\s\S]*?[^\s_])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^_([^\s_<][\s\S]*?[^\s])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^\*([^\s<"][\s\S]*?[^\s\*])\*(?!\*|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^\*([^\s*"<\[][\s\S]*?[^\s])\*(?!\*)/

Old:

/^_([^\s_])_(?!_)|^\*([^\s*"<\[])\*(?!\*)|^_([^\s][\s\S]*?[^\s_])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^_([^\s_][\s\S]*?[^\s])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^\*([^\s"<\[][\s\S]*?[^\s*])\*(?!\*)|^\*([^\s*"<\[][\s\S]*?[^\s])\*(?!\*)/

@UziTech UziTech requested a review from davisjam March 21, 2019 18:42
@UziTech
Copy link
Member

UziTech commented May 21, 2019

@x13machine could you rebase this so we could get it merged?

@x13machine
Copy link
Contributor Author

Ok I'll do that today

@x13machine
Copy link
Contributor Author

x13machine commented May 21, 2019

How do I fix the errors? 😕

@UziTech
Copy link
Member

UziTech commented May 21, 2019

looks like example 455 should be "shouldFail": true and the "shouldFail" line should be removed on example 418 and 477

Also move the new test files to /test/specs/new/

test/new/em_list_links.html Outdated Show resolved Hide resolved
test/new/em_list_links.md Outdated Show resolved Hide resolved
@UziTech
Copy link
Member

UziTech commented May 22, 2019

I'm not sure why the latest commit didn't trigger travis to run the tests, but I ran it locally and everything passed.

@UziTech UziTech requested review from styfle and joshbruce May 22, 2019 17:52
@UziTech
Copy link
Member

UziTech commented May 22, 2019

This still doesn't fix *[link*](url* but that can be a separate PR.

marked demo
commonmark demo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants